[SPARK-37636][SQL] Migrate CREATE NAMESPACE to use V2 command by default by imback82 · Pull Request #35113 · apache/spark

imback82 · 2022-01-06T05:45:13Z

What changes were proposed in this pull request?

This PR proposes to use V2 commands as default as outlined in SPARK-36588, and this PR migrates CREATE NAMESPACE to use v2 command by default.

Note that the work to tests covering both v1/v2 were done in #35093.

Why are the changes needed?

It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands.

Does this PR introduce any user-facing change?

The error message will be slightly different if namespace already exists when v2 command is run against v1 catalog:
v1 command: Database 'db' already exists"
vs.
v2 command: Namespace 'db' already exists"

Also, the error message will be slightly different if namespace already exists when v1 command is run against Hive catalog:
Before: Database db already exists"
vs.
After: Database 'db' already exists"

How was this patch tested?

Existing *CreateNamespaceSuite tests cover this PR's change.

imback82 · 2022-01-06T05:46:06Z

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

-        if isSessionCatalog(catalog) =>
-      if (name.length != 1) {
-        throw QueryCompilationErrors.invalidDatabaseNameError(name.quoted)
-      }


This check is handled in DatabaseNameInSessionCatalog.

imback82 · 2022-01-06T05:47:10Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/v1/CreateNamespaceSuite.scala

    with command.TestsV1AndV2Commands {
  override def namespace: String = "db"
-  override def notFoundMsgPrefix: String = "Database"
+  override def notFoundMsgPrefix: String = if (runningV1Command) "Database" else "Namespace"


We need to update the test now that v1 catalogs are run against both v1 and v2 commands (after adding if conf.useV1Command) .

shall we move this to the base class as well?

imback82 · 2022-01-06T05:47:24Z

cc @cloud-fan

cloud-fan · 2022-01-06T06:15:37Z

sql/core/src/test/scala/org/apache/spark/sql/execution/command/TestsV1AndV2Commands.scala

      super.test(testName, testTags: _*) {
+        // Need to set command version inside this test function so that
+        // the correct command version is available in each test.
+        setCommandVersion()


sorry I'm confused. We set the version right before super.test(....), do you mean it's being set again during super.test(....)?

This def test doesn't run testFun when it's invoked; it only registers the test). So by the time, testFunc is actually run, _version will always be set to "V2" (useV1Command == false). So we need to capture this inside the lambda that's passed into super.test.

Also, note that we need to call setCommandVersion() before calling super.test because super.test being called is utilizing the commandVersion to set the right test name:

spark/sql/core/src/test/scala/org/apache/spark/sql/execution/command/DDLCommandTestUtils.scala

Lines 56 to 57 in 527e842

val testNamePrefix = s"$command using $catalogVersion catalog $commandVersion command"

super.test(s"$testNamePrefix: $testName", testTags: _*)(testFun)

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

cloud-fan · 2022-01-10T08:57:03Z

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala

+          "org.apache.hadoop.hive.metastore.api.AlreadyExistsException")
+        && exception.getMessage.contains(
+          s"Database ${dbDefinition.name} already exists")) {
+      Some(new DatabaseAlreadyExistsException(dbDefinition.name))


Can we test it in the VersionsSuite, by updating the existing $version: createDatabase test? We need to make sure this wrap exception logic works for all hive versions.

Updated the test. Thanks!

cloud-fan · 2022-01-11T14:44:41Z

thanks, merging to master!

### What changes were proposed in this pull request? This PR proposes to use V2 commands as default as outlined in [SPARK-36588](https://issues.apache.org/jira/browse/SPARK-36588), and this PR migrates `CREATE NAMESPACE` to use v2 command by default. Note that the work to tests covering both v1/v2 were done in apache#35093. ### Why are the changes needed? It's been a while since we introduced the v2 commands, and it seems reasonable to use v2 commands by default even for the session catalog, with a legacy config to fall back to the v1 commands. ### Does this PR introduce _any_ user-facing change? The error message will be slightly different if namespace already exists when v2 command is run against v1 catalog: v1 command: `Database 'db' already exists"` vs. v2 command: `Namespace 'db' already exists"` Also, the error message will be slightly different if namespace already exists when v1 command is run against Hive catalog: Before: `Database db already exists"` vs. After: `Database 'db' already exists"` ### How was this patch tested? Existing *CreateNamespaceSuite tests cover this PR's change. Closes apache#35113 from imback82/migrate_create_namespace. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…e/drop database to HiveClientImpl ### What changes were proposed in this pull request? #35113 introduced `HiveExternalCatalog.withClientWrappingException` to to wrap a Hive exception to a Spark exception. However, there was a limitation of testing it against different Hive versions as outlined in #35173 (comment). This PR proposes to revert `HiveExternalCatalog.withClientWrappingException` and move wrapping logic to `HiveClientImpl`. ### Why are the changes needed? 1. To make testing against different Hive versions better. 2. For Hive 0.12, the wrapping logic was not working correctly for `dropDatabase` because the message was not matching. ### Does this PR introduce _any_ user-facing change? Yes, for Hive 0.12, the exception message will now be consistent with other versions. Before: `InvalidOperationException(message:Database db is not empty)` After: `Cannot drop a non-empty database: db. Use CASCADE option to drop a non-empty database` ### How was this patch tested? Added a new test coverage. Closes #35173 from imback82/drop_db_withClientWrappingException. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

…e/drop database to HiveClientImpl ### What changes were proposed in this pull request? apache#35113 introduced `HiveExternalCatalog.withClientWrappingException` to to wrap a Hive exception to a Spark exception. However, there was a limitation of testing it against different Hive versions as outlined in apache#35173 (comment). This PR proposes to revert `HiveExternalCatalog.withClientWrappingException` and move wrapping logic to `HiveClientImpl`. ### Why are the changes needed? 1. To make testing against different Hive versions better. 2. For Hive 0.12, the wrapping logic was not working correctly for `dropDatabase` because the message was not matching. ### Does this PR introduce _any_ user-facing change? Yes, for Hive 0.12, the exception message will now be consistent with other versions. Before: `InvalidOperationException(message:Database db is not empty)` After: `Cannot drop a non-empty database: db. Use CASCADE option to drop a non-empty database` ### How was this patch tested? Added a new test coverage. Closes apache#35173 from imback82/drop_db_withClientWrappingException. Authored-by: Terry Kim <yuminkim@gmail.com> Signed-off-by: Wenchen Fan <wenchen@databricks.com>

initial commit

e6b17c1

github-actions bot added the SQL label Jan 6, 2022

imback82 commented Jan 6, 2022

View reviewed changes

cloud-fan reviewed Jan 6, 2022

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala Outdated Show resolved Hide resolved

gengliangwang reviewed Jan 6, 2022

View reviewed changes

sql/core/src/main/scala/org/apache/spark/sql/catalyst/analysis/ResolveSessionCatalog.scala Show resolved Hide resolved

address PR comments

57179bc

imback82 commented Jan 7, 2022

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 7, 2022

View reviewed changes

sql/hive/src/test/scala/org/apache/spark/sql/hive/execution/command/CreateNamespaceSuite.scala Show resolved Hide resolved

imback82 added 2 commits January 7, 2022 09:48

address PR comment

4eabbb3

address PR comments

20ac17f

cloud-fan reviewed Jan 10, 2022

View reviewed changes

sql/hive/src/main/scala/org/apache/spark/sql/hive/HiveExternalCatalog.scala Outdated Show resolved Hide resolved

cloud-fan reviewed Jan 10, 2022

View reviewed changes

imback82 added 2 commits January 10, 2022 10:28

address comments

ed5c96b

Simplify

044d7e5

cloud-fan approved these changes Jan 11, 2022

View reviewed changes

cloud-fan closed this in 1ed0ff3 Jan 11, 2022

imback82 mentioned this pull request Jan 11, 2022

[SPARK-37636][SQL][FOLLOW-UP] Move handling Hive exceptions for create/drop database to HiveClientImpl #35173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[SPARK-37636][SQL] Migrate CREATE NAMESPACE to use V2 command by default#35113

[SPARK-37636][SQL] Migrate CREATE NAMESPACE to use V2 command by default#35113
imback82 wants to merge 6 commits intoapache:masterfrom
imback82:migrate_create_namespace

imback82 commented Jan 6, 2022 •

edited

Loading

Uh oh!

imback82 Jan 6, 2022

Uh oh!

imback82 Jan 6, 2022

Uh oh!

cloud-fan Jan 10, 2022

Uh oh!

imback82 Jan 10, 2022

Uh oh!

imback82 commented Jan 6, 2022

Uh oh!

cloud-fan Jan 6, 2022

Uh oh!

imback82 Jan 7, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cloud-fan Jan 10, 2022

Uh oh!

imback82 Jan 10, 2022

Uh oh!

cloud-fan commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

	val testNamePrefix = s"$command using $catalogVersion catalog $commandVersion command"
	super.test(s"$testNamePrefix: $testName", testTags: _*)(testFun)

Conversation

imback82 commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes were proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user-facing change?

How was this patch tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imback82 commented Jan 6, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cloud-fan commented Jan 11, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

imback82 commented Jan 6, 2022 •

edited

Loading